-
Notifications
You must be signed in to change notification settings - Fork 221
Adding incremental Frontend Server-compatible DDC builders + supporting infrastructure #4240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…r fine modules for hot reload
PR HealthChangelog Entry ❗
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
Thanks! This will take me a little work to digest :) I guess I can get to it in 1-2 days. It definitely makes sense to expand support for testing. The details are hard :) ... in particular, I would like to avoid exposing any So I will see if I can come up with a slightly differently suggestion. Currently One possibility would be to go all the way to real builds, I recently added a new type of integration test that I'm pretty happy with
I'll give it some thought :) |
|
@davidmorgan Thanks! It's a huge change, so please take your time. I wasn't able to configure it in a prettier way since I'm flying out for two weeks. I'm highly unopinionated wrt testing, but it was definitely a struggle to get @biggs0125 The SCCs here operate on a the import-graph level. DDC's old module system requires that cyclic dependencies be 'unified' into a single module. Both the Frontend Server and build_runner doing this non-deterministically might cause With SCCs enabled, Re: failures. I think some tests are failing due to API changes without pinning new versions? I'll add those (maybe in a separate PR) if the current approach looks sound. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good, a few comments re: scratch_space.
Re: testing :)
You sort of don't need to pass assetGraph
, it's serialized to the readerWriter
and the next build will read it if it's there.
But, the next build will discard the asset graph if any builders changed, so you have to pass the exact same builders each time, which means you will get a full build the first time you call it anyway.
You commented with build.yaml
ordering is not respected, and that's true. Adding workarounds for that is not too pretty, it's getting very close to a real build but now with quite a lot of configuration that doesn't exactly match a real build.
Would you be up for trying a different way of testing that is a real build?
I added tests recently that make it easy to set up some packages, run build_runner
for real and check the output. Since you want persistent processes, watch
mode would make sense, so something like:
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file |
There is an example web build using the same test infra here
The test infra is currently internal to build_runner
, mainly build_runner/test/common/build_runner_tester.dart, if it looks useful let's just hack around that for now and I'll work out how best to clean it up, I guess adding to build_test is one option.
String get generatedOutputDirectory => '$cacheDirectoryPath/generated'; | ||
|
||
/// Relative path to the cache directory from the root package dir. | ||
const cacheDir = '.dart_tool/build'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where this is used, remove?
/// | ||
/// This must be set before any asset builders run when compiling with DDC and | ||
/// hot reload. | ||
late AssetId entrypointAssetId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look particularly related to ScratchSpace
, which I see is used by a few other builders.
I guess the problem here is of passing configuration between builders?
Maybe a new Resource
class for that? It could also go in scratch_space
.
final Directory tempDir; | ||
|
||
/// Holds all files that have been locally modified in this build. | ||
final changedFilesInBuild = <AssetId>{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be private and expose an Iterable? Maybe a read-only view if Set
is needed?
To avoid any possibility of corrupted shared state if a builder thinks it's fun to write to it :)
/// Copies [assetIds] to [tempDir] if they don't exist, using [reader] to | ||
/// read assets and mark dependencies. | ||
/// | ||
/// Locally updated assets will be recorded in [changedFilesInBuild]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest making this and the comment for changedFilesInBuild
more precise, e.g. here
Assets that have changed since the last time they were seen by ensureAssets
are added to changedFilesInBuild
.
And for changedFilesInBuild, maybe: Assets that changed between calls to ensureAssets
. Cleared at the end of every build.
@@ -1 +1 @@ | |||
// Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think scratch_space
needs a new version number and then anything that needs what you're adding here should get a min dep onto that version?
Glues FES into build_runner, which is our first step towards hot reload + build_runner.
The high level workflow is:
build_runner
(hot-reload ready).Major changes:
DdcFrontendServerBuilder
to our set of DDC builders (enabled via theweb-hot-reload
config). This builder keeps aPersistentFrontendServer
instance alive across rebuilds. Compile/recompile requests are queued via aFrontendServerProxyDriver
resource.scratch_space
to record both 1) the main app entrypoint and 2) updated local files from theentrypoint_marker
builder and themodule_builder
builder respectively. These are side effects that break certain stateful 'guarantees' of standard build_runner execution. Theentrypoint_marker
builder runs before any of the downstream DDC builders and finds the web entrypoint, as Frontend Server must receive the same entrypoint on every compilation request.build_runner
be disabled.Test changes:
build_test
to permit incremental builds. This involves passing the asset graph + asset reader/writer across build results and only performing cleanup operations after a series of rebuilds.build_test
doesn't supportruns_before
and other ordering rules inbuild.yaml
, so the above changes allows a kind of imperative ordering, which is important for testingentrypoint_marker
.Minor changes:
scratch_space
so that rebuilds only retain modified files.scratch_space
package_config.json
specs (packageUri
androotUri
). The previous values didn't seem to make sense to me, but I'm also not familiar with how that's standardized inscratch_space
.file
anduuid
deps tobuild_modules
.Currently doesn't support live-reloading (functionality appears to have been broken a while ago). This'll be added in an upcoming change and permit
webdev
-like auto-hot-reload on save (on top of manual).Enable this by adding the following to a project's
build.yaml
: